Potential fix for code scanning alert no. 16: Uncontrolled data used in path expression#439
Conversation
…in path expression Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses a security vulnerability (code scanning alert #16) related to path traversal in the static file serving functionality. The fix replaces a simple startswith check with a more robust path validation approach using os.path.realpath() for normalization and os.path.commonpath() for containment verification. This prevents attackers from using path traversal techniques (e.g., ../../../etc/passwd) or symlinks to access files outside the intended static directory.
Changes:
- Normalize the static directory path using
os.path.realpath()to resolve symlinks and relative paths - Normalize the requested file path using
os.path.realpath()to prevent path traversal attacks - Replace the
startswithsecurity check withos.path.commonpath()for more robust path containment validation
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if os.path.commonpath([static_dir, full_path]) != static_dir: | ||
| self._send_error_response(403, 'Forbidden') | ||
| return | ||
|
|
There was a problem hiding this comment.
The os.path.commonpath function can raise a ValueError on Windows when paths are on different drives (e.g., C:\ and D:). While unlikely in this context (both paths should be on the same drive), this could cause the entire method to fail instead of gracefully returning a 403 Forbidden response. Consider wrapping the commonpath call in a try-except block or using an alternative approach like checking if the normalized full_path starts with the normalized static_dir followed by os.sep.
| if os.path.commonpath([static_dir, full_path]) != static_dir: | |
| self._send_error_response(403, 'Forbidden') | |
| return | |
| try: | |
| common_path = os.path.commonpath([static_dir, full_path]) | |
| except ValueError: | |
| # Paths may be on different drives or otherwise incompatible; deny access. | |
| self._send_error_response(403, 'Forbidden') | |
| return | |
| if common_path != static_dir: | |
| self._send_error_response(403, 'Forbidden') | |
| return | |
| # Security check: ensure file is within static directory | ||
| if not os.path.abspath(full_path).startswith(static_dir): | ||
| if os.path.commonpath([static_dir, full_path]) != static_dir: | ||
| self._send_error_response(403, 'Forbidden') | ||
| return |
There was a problem hiding this comment.
The security fix for path traversal lacks test coverage. Consider adding tests to verify that path traversal attempts (e.g., requests to '/static/../../../etc/passwd', '/static/..\..\..\windows\system32\config\sam') are properly blocked and return 403 Forbidden. This would help prevent regressions and validate the fix works correctly across different operating systems.
Potential fix for https://github.com/kimocoder/wifite2/security/code-scanning/16
In general, the fix is to normalize the constructed file path and ensure it is strictly contained within the intended static root directory before using it, using a robust comparison (for example,
os.path.commonpathor a normalized prefix comparison). This avoids directory traversal where attackers try inputs like/static/../../../../etc/passwdor attempt to exploit symlinks.For this specific code, the best minimally invasive fix is:
static_dirto an absolute, normalized path once in_serve_static_file.full_pathusingos.path.join(static_dir, file_path)as before.full_pathwithos.path.realpath(or at leastos.path.normpath) to collapse..and follow symlinks.startswithcheck with a safer check usingos.path.commonpath([static_dir, full_path]) == static_dir. This ensuresfull_pathis insidestatic_dir, even if the attacker tries path traversal characters or absolute paths.full_pathfor the existence check and the file open.Concretely, within
wifite/attack/portal/server.py:_serve_static_file, update howstatic_dirandfull_pathare computed.with logic that:
static_dirviaos.path.realpath, andfull_path = os.path.realpath(os.path.join(static_dir, file_path)),os.path.commonpath([static_dir, full_path]) == static_dir.No new helper methods are strictly necessary; the existing imports already include
os, so no additional imports are required. All other behavior (caching, content type selection, responses) remains unchanged.Suggested fixes powered by Copilot Autofix. Review carefully before merging.